@aulemahal this is super cool -- thanks for starting on it!

To give some feedback on your two points:

1. Yes, it was an intentional choice to use microseconds as the unit for asi8 on the CFTimeIndex. cftime dates have microsecond precision, and this allows us to represent dates from a much longer time range than pd.Timestamp objects, which have nanosecond precision.

   • You noted this as an option toward the end of your post, and indeed I would actually encourage you to vendor/re-write pandas's _FrequencyInferrer class such that it is specifically designed for cftime dates and microsecond precision. This is for a couple reasons:

     • Multiplying asi8 by 1000 can lead to problems due to integer overflow, e.g. for frequencies greater than ~292 years:

     In [2]: times = xr.cftime_range("2000", periods=10, freq="106945D")
     
     In [3]: xr.infer_freq(times)
     Out[3]: '9240048S'
     

     • _FrequencyInferrer is private API in pandas, which means that they could change and break things we might rely on without warning. I've had a look at pandas's implementation and it doesn't seem to depend too much on other internal functionality to pandas -- in fact you've already implemented some of the more significant internal functions it depends on, build_field_sarray and month_position_check -- which means that hopefully it shouldn't be too onerous to copy the rest over.

2. Given that I am recommending you do not subclass _FrequencyInferrer, I think this is no longer super relevant (i.e. use cftime.datetime.dayofwk in your implementation where needed). I will note that we do not currently implement business-related or weekly cftime offset objects, which will limit the utility of inferring those kinds of frequencies for the time being, but you're definitely not required to add those in this PR!

Thanks @aulemahal, this is looking really nice. I made a first pass of a review.

I removed outputs that wouldn't match offsets defined in xarray: weekly frequencies, week-of-month (WOM) and all business-related anchors. It wouldn't be difficult to add them later on if need be and if cftime_offsets.py defines them. Note that infer_freq is quite independent of the rest of xarray, this choice was purely so that the behavior matches the rest of xr.

Yeah this is totally fine. No one so far has requested these types of frequencies for cftime objects, and it sort of makes sense. While we can somewhat arbitrarily define them for non-standard calendars, the distinction between weekdays and weekends generally only really matters for the kinds of data that would be indexed with a standard calendar.

I expect the performance of xr.infer_freq to be slightly worst than pandas' since they use optimized and compiled code whereas I only used common numpy and xarray stuff. However, I assume that this part of the workflow is far from being the bottleneck of most xarray usecases...

It's indeed slower for cftime objects, but I think much of that is due to the slowness of CFTimeIndex.asi8 compared to DatetimeIndex.asi8 (not any code you wrote in this PR!). I agree this is likely not a bottleneck for most xarray use-cases.

Thanks for all the updates! I think this is almost ready to go from my perspective. I just have a few more very minor requests for tests, based on the latest coverage report:

• Test that a non-monotonic index results in None returned as a frequency.
• Test that an index with non-unique values results in None returned as a frequency.
• Test that an index with more than one monthly delta results in None as a returned frequency, e.g. ["2000-01-01", "2000-02-01", "2000-04-01"].
• Test that an index with more than one quarterly delta results in None as a returned frequency, e.g. ["2000-01-01", "2000-04-01", "2000-10-01"].

Finally could you add an issue requesting that microsecond and millisecond cftime offsets be added? Those would be straightforward to add for someone motivated, but are not required as part of this PR.

Looks great @aulemahal — I’ll merge this in a few days if there are no further comments.